-
Notifications
You must be signed in to change notification settings - Fork 38.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restructure unit tests for more cert/keys. #45107
Restructure unit tests for more cert/keys. #45107
Conversation
fb693d0
to
b88fcdf
Compare
/release-note-none |
/lgtm no functional changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple nits...
|
||
func TestNewManagerNoRotation(t *testing.T) { | ||
cert, err := tls.X509KeyPair([]byte(certificateData), []byte(privateKeyData)) | ||
cert, err := tls.X509KeyPair(testCertificates[0].certificatePEM, testCertificates[0].keyPEM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love having the hardcoded [0]
and [1]
. Consider using named values instead (or constants for indexing).
@@ -278,7 +280,9 @@ func TestLoadFile(t *testing.T) { | |||
}() | |||
|
|||
pairFile := filepath.Join(dir, "kubelet-pair.pem") | |||
if err := ioutil.WriteFile(pairFile, []byte(certificateData+"\n"+privateKeyData), 0600); err != nil { | |||
data := append(testCertificates[0].certificatePEM, []byte("\n")...) | |||
data = append(data, testCertificates[0].keyPEM...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This can be shortened to:
data := append(testCertificates[0].certificatePEM, byte("\n"), testCertificates[0].keyPEM...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get that to work: https://play.golang.org/p/E3lDLT5vHw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, once I use ...
on the last parameter, the compiler won't permit anything in the middle. https://play.golang.org/p/Xg-z__MhW1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, should be byte('\n')
(single quotes). I forgot that ...
parameters can't be mixed with other params though, so I guess you still need the 2 lines (or nested append)
@@ -408,7 +412,9 @@ func TestCurrentPairFile(t *testing.T) { | |||
} | |||
}() | |||
pairFile := filepath.Join(dir, prefix+"-pair.pem") | |||
if err := ioutil.WriteFile(pairFile, []byte(certificateData+"\n"+privateKeyData), 0600); err != nil { | |||
data := append(testCertificates[0].certificatePEM, []byte("\n")...) | |||
data = append(data, testCertificates[0].keyPEM...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
b88fcdf
to
0e92319
Compare
0e92319
to
ac171f6
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcbsmpsn, mikedanese, timstclair
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue |
Just changing the unit tests so there is multiple cert/key pairs to be used.
No functional change, no new tests. Follow on PRs will make more use
of the multiple cert/key pairs.